Skip to content

Track passing/failing tests in fiber#8169

Merged
sophiebits merged 2 commits intofacebook:masterfrom
sophiebits:fiber-test-tracking
Nov 1, 2016
Merged

Track passing/failing tests in fiber#8169
sophiebits merged 2 commits intofacebook:masterfrom
sophiebits:fiber-test-tracking

Conversation

@sophiebits
Copy link
Copy Markdown
Collaborator

Run scripts/fiber/record-tests to re-record, then check git diff to see what you changed.

Copy link
Copy Markdown
Contributor

@sebmarkbage sebmarkbage left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why does this need a work around for the test failure when the other Fiber test runner doesn't?

The strategy seems sound but it looks like this causes a Travis error. Something not found.

return Object.assign({}, flags, {
useFiber: !!process.env.REACT_DOM_JEST_USE_FIBER,
});
});
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't we somehow have a flag already?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah but it requires changing ReactDOMFeatureFlags.js which feels safe on Travis and not as safe if you might have local changes.

@sophiebits
Copy link
Copy Markdown
Collaborator Author

The other one failed on my machine too.

@sebmarkbage
Copy link
Copy Markdown
Contributor

ok

Comment thread package.json Outdated
"gulp-util": "^3.0.7",
"gzip-js": "~0.3.2",
"jest": "^15.1.1",
"jest-cli": "^15.1.1",
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is the jest package just an alias for jest-cli? Only one is needed I think.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

jest seemingly just delegates to jest-cli but it does have the same exports so (y).

Run scripts/fiber/record-tests to re-record, then check git diff to see what you changed.
@sophiebits sophiebits merged commit 08b4cc5 into facebook:master Nov 1, 2016
Comment thread .travis.yml
scripts/fiber/record-tests
git --no-pager diff scripts/fiber
FIBER_TESTS_STATUS=$(git status --porcelain scripts/fiber)
test -z "$FIBER_TESTS_STATUS"
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this mean that any time anyone adds a passing test, unrelated to Fiber, in a PR we will get a Travis error on the PR?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That was my intention. I figured it was good to know whether your new test passes in Fiber. Not good?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's good for the team but might be noisy for third-party PR. Not too worried about it though. Let's see how it works out.

@sophiebits
Copy link
Copy Markdown
Collaborator Author

@aweary Heads up if you didn't see: everyone needs to run scripts/fiber/record-tests when adding/removing tests (or breaking/fixing them in the fiber reconciler) or else CI will fail now. This way it's really obvious whether tests pass or fail in fiber when looking at a diff.

@aweary
Copy link
Copy Markdown
Contributor

aweary commented Nov 2, 2016

@spicyj thanks, I didn't see this. I'll make sure to do this 👍

acusti pushed a commit to brandcast/react that referenced this pull request Mar 15, 2017
* Work around jest toEqual bug in ReactTreeTraversal

![image](https://cloud.githubusercontent.com/assets/6820/19879640/1cd7595a-9fb2-11e6-94ac-8c38bdfc90d3.png)

* Track passing/failing tests in fiber

Run scripts/fiber/record-tests to re-record, then check git diff to see what you changed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants